perf: fix FindIncludesInContent regression (~18% slower)#29949
Merged
perf: fix FindIncludesInContent regression (~18% slower)#29949
Conversation
…-scan fast path
Three targeted optimizations to recover the 17.9% regression:
1. hasDirectiveMarker: single-scan helper using IndexByte (SIMD) to find
'@' or '{', avoiding three separate Contains passes for the no-include
fast path. Content is now traversed at most once before the line scan.
2. parseIncludePath: first-byte guard skips all three HasPrefix comparisons
for the overwhelming majority of regular-text lines that don't start
with '@' or '{'.
3. findIncludesInContent: manual IndexByte('\n') loop replaces
strings.Lines (iter.Seq), eliminating the yield-function call overhead
per line and stripping the trailing '\n' before parseIncludePath sees it.
Benchmark (5 s, AMD EPYC):
Before: 194 ns/op → After: ~174 ns/op (~10% improvement)
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7cb7b62b-a88d-4dc4-b5ed-0437bae12238
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix regression in FindIncludesInContent performance
perf: fix FindIncludesInContent regression (~18% slower)
May 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Improves performance of include-directive detection/parsing in findIncludesInContent by reducing overhead on the common “no directives” case and by avoiding iterator/yield costs in line scanning.
Changes:
- Adds a pre-scan (
hasDirectiveMarker) to skip line-by-line parsing when no directive markers are present. - Replaces
strings.Linesiteration with a manualIndexByte('\n')loop to reduce per-line overhead. - Adds a first-byte guard in
parseIncludePathto avoid unnecessary prefix checks for non-directive lines.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/remove_command.go | Optimizes include marker detection and line scanning to recover findIncludesInContent performance. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
Comment on lines
+393
to
+413
| // Find next '@' or '{'—both are rare in workflow prose, so IndexByte skips | ||
| // large stretches of uninteresting bytes via SIMD on supported platforms. | ||
| atIdx := strings.IndexByte(content[i:], '@') | ||
| braceIdx := strings.IndexByte(content[i:], '{') | ||
| if atIdx < 0 && braceIdx < 0 { | ||
| return false | ||
| } | ||
| if atIdx >= 0 && (braceIdx < 0 || atIdx <= braceIdx) { | ||
| pos := i + atIdx | ||
| rest := content[pos:] | ||
| if strings.HasPrefix(rest, "@include") || strings.HasPrefix(rest, "@import") { | ||
| return true | ||
| } | ||
| i = pos + 1 | ||
| } else { | ||
| pos := i + braceIdx | ||
| if strings.HasPrefix(content[pos:], "{{#import") { | ||
| return true | ||
| } | ||
| i = pos + 1 | ||
| } |
Collaborator
|
@Copiloy review all comments |
Collaborator
|
@copilot review all comments |
Fixes the double-scan issue flagged in code review: the previous
implementation called strings.IndexByte twice per loop iteration
(once for '@' and once for '{'), scanning content twice end-to-end
for the no-marker case.
Using strings.IndexAny("@{") finds the next candidate byte in a
single pass, ensuring content is traversed at most once.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1cd5dff8-a199-4c49-88ec-f11cbd94064b
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FindIncludesInContentregressed ~17.9% (2,409 → 2,842 ns/op) due to overhead in the per-line iterator and redundant prefix comparisons on non-directive lines.Changes
Single-scan fast path (
hasDirectiveMarker) — replaces three sequentialstrings.Containscalls with one forward pass using SIMD-friendlyIndexByteto locate@/{candidates, then checks prefixes only at those positions.First-byte guard in
parseIncludePath— short-circuits before all threeHasPrefixcomparisons for the common case of regular prose lines:Manual
IndexByteline scan — replacesstrings.Lines(iter.Seqwith a yield-function call per line) with a directstrings.IndexByte(remaining, '\n')loop, eliminating per-line call overhead and the trailing\nthatstrings.Linesincludes in each yielded string.Benchmark (5 s, AMD EPYC): ~194 ns/op → ~174 ns/op (~10% improvement locally; targets recovery of the full regression on the benchmark host).